feat: Added part_size param to transfer manager for MultipartUploader.#3118
feat: Added part_size param to transfer manager for MultipartUploader.#3118yurguis wants to merge 4 commits into
Conversation
|
Hi @yurguis, thanks for this contribution. Would you please be able to add a test case for this change? Thanks! |
…com:yurguis/aws-sdk-php into feat-add-partsize-param-to-transfer-manager
|
Hi @yenfryherrerafeliz, just updated the PR with a test case for this change. Let me know if there is anything else needed. Thanks! |
| $this->partSize = isset($options['part_size']) | ||
| ? $options['part_size'] | ||
| : MultipartUploader::PART_MIN_SIZE; |
There was a problem hiding this comment.
There should be some validation here against MultipartUploader::PART_MIN_SIZE and MultipartUploader::PART_MAX_SIZE. If it's less than the min size or greater than the max size, throw an InvalidArgumentException
There was a problem hiding this comment.
part_size is missing a docblock entry in the "The options array can contain the following key value pairs..." section
There was a problem hiding this comment.
Needs a couple more tests following the validation suggestions above
| $t->transfer(); | ||
| rewind($res); | ||
| $output = stream_get_contents($res); | ||
| $this->assertStringNotContainsString("Transferring $filename -> s3://foo/bar/large.txt (UploadPart) : Part=3", $output); |
There was a problem hiding this comment.
Test asserts only the absence of a Part=3 line. better to assert positively or via mock command capture (Middleware::tap())
|
Hi @yurguis, Sorry for the long delay- Just left a couple of comments |
3 participants
Description of changes:
Adds a new parameter to transfer manager's options array (
part_size) to allow to specify the part size for multipart uploads.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.